Fix #14273: Prevent race condition crash in FPRTraceBackgroundActivityTracker #15382
Conversation
Replace async notification observer registration with synchronous registration to eliminate timing-dependent crashes when objects are deallocated quickly. Fixes firebase#14273
|
@visumickey Created PR Here |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical race condition in FPRTraceBackgroundActivityTracker by switching from asynchronous to synchronous notification observer registration. The solution is robust, handling initialization from both main and background threads correctly to prevent deadlocks. The accompanying tests are comprehensive, not only verifying the fix under race conditions but also including a clear reproduction case for the original bug, which is excellent for future reference. The code is clean and the changes are well-explained. I have one minor suggestion in the tests to improve clarity.
FirebasePerformance/Tests/Unit/FPRTraceBackgroundActivityTrackerTest.m
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
Excellent work on this pull request. You've correctly identified and fixed a critical race condition in FPRTraceBackgroundActivityTracker by replacing the asynchronous observer registration with a synchronous approach. The use of if ([NSThread isMainThread]) followed by dispatch_sync is a robust pattern that ensures thread safety and prevents the deallocation issue. The new unit tests are thorough and provide great coverage for the fix, including a specific test to reproduce and verify the absence of the race condition. I have one minor suggestion regarding code style for better maintainability.
FirebasePerformance/Sources/AppActivity/FPRTraceBackgroundActivityTracker.m
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a race condition crash in FPRTraceBackgroundActivityTracker by replacing asynchronous notification observer registration with a synchronous approach. The implementation correctly handles object initialization from both main and background threads, and the new unit tests are comprehensive, verifying the fix under various conditions, including rapid allocation/deallocation cycles. I have one minor suggestion to consolidate a redundant test case, but overall this is an excellent fix.
FirebasePerformance/Tests/Unit/FPRTraceBackgroundActivityTrackerTest.m
Outdated
Show resolved
Hide resolved
|
@visumickey you can review from here, I added some of the Gemini suggestions (though idk if they where any good) lmk if anything! |
FirebasePerformance/Sources/AppActivity/FPRTraceBackgroundActivityTracker.m
Outdated
Show resolved
Hide resolved
FirebasePerformance/Tests/Unit/FPRTraceBackgroundActivityTrackerTest.m
Outdated
Show resolved
Hide resolved
Capture strong self during async observer registration
|
@visumickey Made new changes and added commit, I also re tested everything and it seems fine 
|
FirebasePerformance/Sources/AppActivity/FPRTraceBackgroundActivityTracker.m
Outdated
Show resolved
Hide resolved
Add a Check before calling registerNotificationObservers to make the approach safer and address a nit pick of a duplicate / similar test
Replace async notification observer registration with synchronous registration
to eliminate timing-dependent crashes when objects are deallocated quickly.
Fixes #14273 (Link)
Discussion
The FPRTraceBackgroundActivityTracker class had a race condition during initialization where notification observers were registered asynchronously using dispatch_async(dispatch_get_main_queue()). This created a timing window where the tracker object could be deallocated before the async block executed, causing crashes when the block tried to call addObserver: with a deallocated self reference.
The crash occurred when:
My solution involves to replace the asynchronous observer registration with synchronous registration to eliminate the race condition entirely
Testing
API Changes